Make cash have all betas enabled when running in development mode #3135

iwiznia · 2021-05-25T20:54:59Z

Details

Fixed Issues

Fixes https://expensify.slack.com/archives/C03TQ48KC/p1621974182298000

Tests

Make the api User_GetBetas return []
Load the app before this change, make sure request money option is not shown in the big plus button
Load the app after this change, make sure request money option is shown in the big plus button

QA Steps

None, since this is just for dev

Tested On

tgolen · 2021-05-25T21:18:39Z

src/libs/Permissions.js

@@ -9,12 +11,14 @@ Onyx.connect({
    callback: val => betas = val || [],
 });

+const isDevelopment = lodashGet(Config, 'ENVIRONMENT', CONST.ENVIRONMENT.DEV) === CONST.ENVIRONMENT.DEV;


I was thinking you should use getEnvironment() from the Environment module so that it will be cross-platform. However, that will also force this code to be async, which will be a pain to refactor. I believe it's only necessary to use getEnvironement() if you care about the difference between production and staging. Since you don't, it might be good to add a method to the Environment module called isDevelopment() which is a synchronous function (and then add method docs to explain why it's OK for it to be synchronous when getEnvironment() has to be async).

Interesting. Why is that method returning a Promise?

Could we alternatively just use the __DEV__ global? It is being set here...

https://github.com/Expensify/Expensify.cash/blob/9a9099592330bc26d06408f34d76128718196eb4/config/webpack/webpack.dev.js#L37-L37

Interesting. Why is that method returning a Promise?

Why indeed, given it is getting a value from an already loaded object. Any objections to change that?

The mobile versions of the environment module need to make a network request to get the info about if it's in staging or production. That's why it needs to be async.

Oh I see now, because only the web app uses the staging env configs and the native apps always use production config, but we need to know if they are TestFlight or google play beta versions.

Oh, true, did not notice that. So what is preferred to add a isDevelopment that's not async or use the __DEV__ global? If the latter, not sure how I grab that constant.

I feel they are different concepts. It just happens that we are running in __DEV__ mode when we are using the development environment. But we could also be running the development environment (i.e. not staging or production) and not be "running in dev mode".

Regardless, I'd think we want the environment in this case since whether you are running in dev mode or not the betas would probably be enabled in the "development environment".

What about solving this from the API side? If PHP sends all in the beta information, then we should be fine. And PHP knows whether we are in DEBUG mode or not.

https://github.com/Expensify/Expensify.cash/blob/e28b50e139c492442ea984c3d48e23be744ceb4e/src/libs/Permissions.js#L17

It is A LOT harder to detect where the request is coming from, from the server than in the client... keep in mind this has to work for contributors that do not point the API to the dev environment, since they don't have one.

tgolen · 2021-05-26T16:26:26Z

Ah, that's right... It wouldn't work well for contributors. OK, scrap that idea then.

…

On Tue, May 25, 2021 at 5:22 PM Ionatan Wiznia ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/libs/Permissions.js <#3135 (comment)> : > @@ -9,12 +11,14 @@ Onyx.connect({ callback: val => betas = val || [], }); +const isDevelopment = lodashGet(Config, 'ENVIRONMENT', CONST.ENVIRONMENT.DEV) === CONST.ENVIRONMENT.DEV; It is A LOT harder to detect where the request is coming from the server... keep in mind this has to work for contributors that do not point the API to the dev environment, since they don't have one. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3135 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB373HQJATRKLJECCETTPQWMFANCNFSM45QGD2VA> .

iwiznia · 2021-05-26T22:20:40Z

Alright, updated

jasperhuangg

Looks pretty straightforward and good to me!

tgolen · 2021-05-27T15:35:32Z

src/libs/Environment/isDevelopment.js

@@ -0,0 +1,12 @@
+import lodashGet from 'lodash/get';


I was trying to figure out why this was in it's own file, and then it dawned on me why you did this (and I totally see the logic). I'm not sure it's what we want though. This doesn't really fit our cross-platform pattern because there is a cross-platform module (Environment) that is sharing non-platform specific methods (isDevelopment).

I think rather we should have Environment be non-platform specific and then making only the pieces that need to be platform-specific (getEnvironment in this case) would be split into their platform files.

I hope that makes sense?

iwiznia · 2021-05-27T21:41:36Z

Updated and retested

tgolen · 2021-05-27T21:45:12Z

src/libs/Environment/Environment.js

+import CONST from '../../CONST';
+import getEnvironment from './getEnvironment';
+
+function isDevelopment() {


This needs a method doc for the return value.

tgolen · 2021-05-27T21:46:30Z

src/libs/Environment/betaChecker/index.js

+ * @returns {Promise}
+ */
+function isBetaBuild() {
+    return new Promise(resolve => resolve(false));


I think this can simply be

Suggested change

return new Promise(resolve => resolve(false));

return Promise.resolve(false);

You'll have to double-check that it works, but I think it does according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/resolve

tgolen · 2021-05-27T21:47:54Z

src/libs/Environment/getEnvironment/index.js

@@ -11,6 +11,4 @@ function getEnvironment() {
    return new Promise(resolve => resolve(lodashGet(Config, 'ENVIRONMENT', CONST.ENVIRONMENT.DEV)));


These could also use a more simple return Promise.resolve()

tgolen · 2021-05-27T21:50:55Z

src/libs/Environment/getEnvironment/index.native.js

@@ -1,7 +1,7 @@
 import lodashGet from 'lodash/get';
 import Config from 'react-native-config';
-import betaChecker from './betaChecker';
-import CONST from '../../CONST';
+import betaChecker from '../betaChecker/index';


Suggested change

import betaChecker from '../betaChecker/index';

import betaChecker from '../betaChecker';

No need for that. The modules are smart enough to figure it out

iwiznia · 2021-05-27T22:00:35Z

Updated

OSBotify · 2021-05-27T22:54:30Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Make cash have all betas enabled when running in development mode

34be6eb

iwiznia requested a review from a team May 25, 2021 20:54

iwiznia self-assigned this May 25, 2021

MelvinBot requested review from marcaaron and removed request for a team May 25, 2021 20:55

tgolen requested changes May 25, 2021

View reviewed changes

Use isDevelopment method from Environment

99a8bfe

iwiznia requested a review from a team as a code owner May 26, 2021 22:20

MelvinBot requested review from jasperhuangg and removed request for a team May 26, 2021 22:20

jasperhuangg previously approved these changes May 27, 2021

View reviewed changes

marcaaron previously approved these changes May 27, 2021

View reviewed changes

tgolen requested changes May 27, 2021

View reviewed changes

iwiznia dismissed stale reviews from marcaaron and jasperhuangg via 256ab7e May 27, 2021 21:31

Change what has native/non native separation

77054aa

iwiznia force-pushed the ionatan_dev_allbetas branch from 256ab7e to 77054aa Compare May 27, 2021 21:41

tgolen requested changes May 27, 2021

View reviewed changes

Address review comments

903ead3

tgolen approved these changes May 27, 2021

View reviewed changes

marcaaron approved these changes May 27, 2021

View reviewed changes

marcaaron merged commit 81eb997 into main May 27, 2021

marcaaron deleted the ionatan_dev_allbetas branch May 27, 2021 22:53

OSBotify mentioned this pull request May 28, 2021

Deploy Checklist: Expensify.cash 2021-05-28 #3199

Closed

27 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make cash have all betas enabled when running in development mode #3135

Make cash have all betas enabled when running in development mode #3135

iwiznia commented May 25, 2021

tgolen May 25, 2021

marcaaron May 25, 2021

iwiznia May 25, 2021

tgolen May 25, 2021

marcaaron May 25, 2021 •

edited

Loading

iwiznia May 25, 2021

marcaaron May 25, 2021

tgolen May 25, 2021

iwiznia May 25, 2021 •

edited

Loading

tgolen commented May 26, 2021 via email

iwiznia commented May 26, 2021

jasperhuangg left a comment

tgolen May 27, 2021

iwiznia commented May 27, 2021

tgolen May 27, 2021

tgolen May 27, 2021

tgolen May 27, 2021

tgolen May 27, 2021

tgolen May 27, 2021

iwiznia commented May 27, 2021

OSBotify commented May 27, 2021

	return new Promise(resolve => resolve(false));
	return Promise.resolve(false);

		@@ -11,6 +11,4 @@ function getEnvironment() {
		return new Promise(resolve => resolve(lodashGet(Config, 'ENVIRONMENT', CONST.ENVIRONMENT.DEV)));

	import betaChecker from '../betaChecker/index';
	import betaChecker from '../betaChecker';

Make cash have all betas enabled when running in development mode #3135

Make cash have all betas enabled when running in development mode #3135

Conversation

iwiznia commented May 25, 2021

Details

Fixed Issues

Tests

QA Steps

Tested On

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron May 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwiznia May 25, 2021 • edited Loading

Choose a reason for hiding this comment

tgolen commented May 26, 2021 via email

iwiznia commented May 26, 2021

jasperhuangg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwiznia commented May 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwiznia commented May 27, 2021

OSBotify commented May 27, 2021

marcaaron May 25, 2021 •

edited

Loading

iwiznia May 25, 2021 •

edited

Loading